Skip to content

[Zeppelin-1561] Improve sync for multiuser environment #1537

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
khalidhuseynov wants to merge 8 commits into from
Closed

[Zeppelin-1561] Improve sync for multiuser environment #1537

khalidhuseynov wants to merge 8 commits into from

Conversation

@khalidhuseynov
Copy link
Member

@khalidhuseynov khalidhuseynov commented Oct 18, 2016 •
edited
Loading

What is this PR for?

apply multi-tenancy for storage sync mechanism

What type of PR is it?

Bug Fix | Improvement

Todos

  • - broadcast on sync
  • - set permissions for pulled notes
  • - add test

What is the Jira issue?

ZEPPELIN-1561

How should this be tested?

Outline the steps to test the PR here.

Screenshots (if appropriate)

green CI

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? no

@khalidhuseynov khalidhuseynov changed the title Improve/sync multiuser [Zeppelin-1561] Improve sync for multiuser environment Oct 18, 2016
anthonycorbacho
anthonycorbacho reviewed Oct 18, 2016
View reviewed changes
Copy link
Contributor

@anthonycorbacho anthonycorbacho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for handeling this case, i added a comment please let me know what your think about it

zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java Outdated
}

private void makePrivate(String noteId, AuthenticationInfo subject) {
if (subject != null && !"anonymous".equals(subject.getUser())) {
Copy link
Contributor

@anthonycorbacho anthonycorbacho Oct 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about creating in AuthenticationInfo a new method called isAnonymous(AuthenticationInfo subject) and move the check inside?

something like

// In AuthenticationInfo.java
 private static final String ANONYMOUS = "anonymous";

 public static boolean isAnonymous(AuthenticationInfo subject) {
   if (subject != null ) {
     LOG.info("Subject is null assuming anonymous");
     return true;
   }
   return ANONYMOUS.equals(subject.getUser(); 
 }

//for the fun
public boolean isAnonymous() {
  return ANONYMOUS.equals(user);
}

//then here you can do something like this in makePrivate

if (AuthenticationInfo.isAnonymous(subject)) {
  // Add maybe meaningful log :)
  return;
}
NotebookAuthorization notebookAuthorization = NotebookAuthorization.getInstance();
Set<String> users = notebookAuthorization.getOwners(noteId);
users.add(subject.getUser());
notebookAuthorization.setOwners(noteId, users);
users = notebookAuthorization.getReaders(noteId);
users.add(subject.getUser());
notebookAuthorization.setReaders(noteId, users);
users = notebookAuthorization.getWriters(noteId);
users.add(subject.getUser());
notebookAuthorization.setWriters(noteId, users);

Copy link
Member Author

@khalidhuseynov khalidhuseynov Oct 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, can be reused to make anonymous consistent in usage over codebase. I'll change now

@khalidhuseynov
Copy link
Member Author

khalidhuseynov commented Oct 18, 2016

@anthonycorbacho addressed in d0a8c3c with some minor changes

ghost
ghost reviewed Oct 18, 2016
View reviewed changes
zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java Outdated
LOG.info("No storages could be initialized, using default {} storage", defaultStorage);
initializeDefaultStorage(conf);
}
//syncOnStart();
Copy link

@ghost ghost Oct 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see syncOnStart method called anywhere. So, i believe you commented the above line by mistake.

Copy link
Member Author

@khalidhuseynov khalidhuseynov Oct 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajarajan-g thanks for pointing to it. actually initial purpose was to delete it, but it was extracted into sub-function and commented out. addressed by deleting it in 1c11c2d

Leemoonsoo
Leemoonsoo reviewed Oct 23, 2016
View reviewed changes
zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java
}
NotebookAuthorization notebookAuthorization = NotebookAuthorization.getInstance();
Set<String> users = notebookAuthorization.getOwners(noteId);
users.add(subject.getUser());
Copy link
Member

@Leemoonsoo Leemoonsoo Oct 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate why 'makePrivate()' routine, which is being used inside of sync() adds current user as a owner, reader, writer?

Copy link
Member Author

@khalidhuseynov khalidhuseynov Oct 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, it sets it when pulling notes from secondary storage to your main one

Copy link
Member

@Leemoonsoo Leemoonsoo Oct 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why it sets when pulling notes from secondary storage to the main one?

For example, what happen if a note permission has a user as a reader, but not as a writer and owner?

Copy link
Member Author

@khalidhuseynov khalidhuseynov Oct 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's pulling, that means there was no such a note in main storage and it's pulling new note from secondary storage. Thus, that new note not supposed to have previous owners, readers, writers. So generally it's same as creating note and only owner should be set. But I also set readers and writers in order other users don't see that new note (make it private).

@anthonycorbacho anthonycorbacho mentioned this pull request Oct 23, 2016
Closed
3 tasks
@khalidhuseynov
Copy link
Member Author

khalidhuseynov commented Oct 23, 2016

@Leemoonsoo your comment makes sense if there's already existing acl for the note. addressed it with test under e41b9b9

@khalidhuseynov khalidhuseynov force-pushed the improve/sync-multiuser branch 3 times, most recently from b5d026c to 0355f26 Compare October 23, 2016 13:24
@khalidhuseynov
Copy link
Member Author

khalidhuseynov commented Oct 23, 2016

CI is green, ready for review

@anthonycorbacho
Copy link
Contributor

anthonycorbacho commented Oct 24, 2016

LGTM merging if no more discussion

@asfgit asfgit closed this in dfbea2e Oct 24, 2016
darionyaphet pushed a commit to darionyaphet/zeppelin that referenced this pull request Oct 27, 2016
@khalidhuseynov @darionyaphet
[Zeppelin-1561] Improve sync for multiuser environment
c99a230 
### What is this PR for?
apply multi-tenancy for storage sync mechanism

### What type of PR is it?
Bug Fix | Improvement

### Todos
* [x] - broadcast on sync
* [x] - set permissions for pulled notes
* [x] - add test

### What is the Jira issue?
 [ZEPPELIN-1561](https://issues.apache.org/jira/browse/ZEPPELIN-1561)

### How should this be tested?
Outline the steps to test the PR here.

### Screenshots (if appropriate)
green CI

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: Khalid Huseynov <[email protected]>

Closes apache#1537 from khalidhuseynov/improve/sync-multiuser and squashes the following commits:

b3e6ed3 [Khalid Huseynov] add userAndRoles
0f2ade7 [Khalid Huseynov] reformat style
bd1a44a [Khalid Huseynov] address comment + test
05afa2a [Khalid Huseynov] remove syncOnStart
b104249 [Khalid Huseynov] add isAnonymous
1a54cc0 [Khalid Huseynov] set perms for pulling notes - make them private
585a675 [Khalid Huseynov] reload, sync and broadcast on login
cd1c3fa [Khalid Huseynov] don't sync on start
pedrozatta pushed a commit to pedrozatta/zeppelin that referenced this pull request Oct 27, 2016
@khalidhuseynov
[Zeppelin-1561] Improve sync for multiuser environment
537c71c 
### What is this PR for?
apply multi-tenancy for storage sync mechanism

### What type of PR is it?
Bug Fix | Improvement

### Todos
* [x] - broadcast on sync
* [x] - set permissions for pulled notes
* [x] - add test

### What is the Jira issue?
 [ZEPPELIN-1561](https://issues.apache.org/jira/browse/ZEPPELIN-1561)

### How should this be tested?
Outline the steps to test the PR here.

### Screenshots (if appropriate)
green CI

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: Khalid Huseynov <[email protected]>

Closes apache#1537 from khalidhuseynov/improve/sync-multiuser and squashes the following commits:

b3e6ed3 [Khalid Huseynov] add userAndRoles
0f2ade7 [Khalid Huseynov] reformat style
bd1a44a [Khalid Huseynov] address comment + test
05afa2a [Khalid Huseynov] remove syncOnStart
b104249 [Khalid Huseynov] add isAnonymous
1a54cc0 [Khalid Huseynov] set perms for pulling notes - make them private
585a675 [Khalid Huseynov] reload, sync and broadcast on login
cd1c3fa [Khalid Huseynov] don't sync on start
@khalidhuseynov khalidhuseynov deleted the improve/sync-multiuser branch November 14, 2016 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@Leemoonsoo Leemoonsoo Leemoonsoo left review comments

@anthonycorbacho anthonycorbacho anthonycorbacho left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@khalidhuseynov @anthonycorbacho @Leemoonsoo